Feat/refactor-bcli #8820
Feat/refactor-bcli #8820
Conversation
plugins/bcli.c
Outdated
| res->output_len = strlen(res->output); | ||
|
|
||
| /* Wait for child to exit */ | ||
| while (waitpid(child, &status, 0) < 0 && errno == EINTR); |
There was a problem hiding this comment.
Might be good to have something like this here:
while (waitpid(child, &status, 0) < 0) {
if (errno == EINTR)
continue;
plugin_err(plugin, "waitpid(%s) failed: %s",
res->args, strerror(errno));
}
There was a problem hiding this comment.
Yeah, added this.
Also I think I forgot to close a from descriptor in wait_and_check_bitcoind and run_bitcoin_cliv. Added this as well.
| * we can attempt to prevent a crash if the 'getchaininfo' function returns | ||
| * a lower height than the one we already know, by waiting for a short period. | ||
| * However, I currently don't have a better idea on how to handle this situation. */ | ||
| u32 *height UNUSED; |
There was a problem hiding this comment.
I think it's fine to leave this as it is and not use the optional last_height parameter. We're planning on getting rid of retries altogether inside bcli and have lightningd solely deal with retrying. So it feels a bit clunky to me to add retrying based on the last_height and ALSO have it be blocking to the rest of the plugin. We were never using it before and I don't think anyone has complained about this (yet) so it's probably not fatal?
| /* As of at least v0.15.1.0, bitcoind returns "success" but an empty | ||
| string on a spent txout. */ |
There was a problem hiding this comment.
I couldn't see this documented in the bitcoin-cli documentation https://developer.bitcoin.org/reference/rpc/gettxout.html , it might be good to test it out on cli to see if it every comes up with something else that's not an empty string but something like the string null?
There was a problem hiding this comment.
That should be alright. I've found an issue raising this.
plugins/bcli.c
Outdated
| static struct command_result *command_err_badjson(struct command *cmd, | ||
| struct bcli_result *res, | ||
| const char *errmsg) | ||
| { | ||
| char *err = tal_fmt(cmd, "%s: bad JSON: %s (%.*s)", | ||
| res->args, errmsg, (int)res->output_len, res->output); | ||
| return command_done_err(cmd, BCLI_ERROR, err, NULL); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this function is actually pretty good to have! Despite the plugin now being synchronous, it doesn't have to die, which plugin_err would do.
| if (res->exitstatus != 0) | ||
| return estimatefees_null_response(cmd); | ||
|
|
||
| tokens = json_parse_simple(res->output, res->output, res->output_len); |
There was a problem hiding this comment.
I found this pattern quite absurd because res->output is not a ctx! But it's used all over bcli in other places so it's not your fault at all! Have asked @rustyrussell about this and if it is intentional.
There was a problem hiding this comment.
Asked him about this, this works okay in these scenarios because the lifetime of the pointers are the same so once the parent pointer is cleaned up any children are also cleaned up at the same time.
dovgopoly
force-pushed
the
feat/refactor-bcli
branch
2 times, most recently
from
2faf7ec to
8512a1d
Compare
dovgopoly
force-pushed
the
feat/refactor-bcli
branch
from
509f2f7 to
78c4f86
Compare
ShahanaFarooqui
force-pushed
the
feat/refactor-bcli
branch
from
78c4f86 to
a8079be
Compare
dovgopoly
force-pushed
the
feat/refactor-bcli
branch
4 times, most recently
from
bbc4e8d to
b3632a5
Compare
Add `command_err_badjson` helper for sync error handling, mirroring the async `command_err_bcli_badjson`. Store args string in `bcli_result` for consistent error messages.
dovgopoly
force-pushed
the
feat/refactor-bcli
branch
from
b3632a5 to
abf4d82
Compare
Also rename command_err_badjson to generic command_err helper, since error messages aren't always about bad JSON (e.g., "command failed" for non-zero exit).
Add check_bitcoin_error to detect errors from bcli plugin and call fatal with a clear message identifying the failing method.
dovgopoly
force-pushed
the
feat/refactor-bcli
branch
from
abf4d82 to
ae11557
Compare
| # We block l3 from seeing close, so it will try to reestablish. | ||
| def no_new_blocks(req): | ||
| return {"error": "go away"} | ||
| return {"error": {"code": -8, "message": "Block height out of range"}} |
There was a problem hiding this comment.
If we revert with another code, it kills lightningd instantly, not making retry anymore.
I think it's expected to return a null response to the lightningd here.
Co-authored-by: ShahanaFarooqui <[email protected]>
Changelog-Changed: bcli plugin now uses synchronous execution, simplifying bitcoin backend communication and improving error handling reliability.
dovgopoly
force-pushed
the
feat/refactor-bcli
branch
from
ae11557 to
dddf995
Compare
2 participants
Closes #8738.
This PR refactors the bcli plugin from async to sync execution. It introduces a new
run_bitcoin_clifunction that blocks until bitcoin-cli completes.All RPC commands are converted to use this synchronous pattern.
The PR also adds a
command_errhelper for consistent error handling, improves waitpid error reporting, fixes fd leaks by properly closing file descriptors, and removes all the now-unused async infrastructure (pending queues, priority scheduling, retry callbacks).Tests are fixed accordingly.